Replace offset shifting animations with palette cycling by morganchristiansson · Pull Request #57 · Return-To-The-Roots/s25edit

morganchristiansson · 2026-06-30T11:05:57Z

Using pos values from GameDataLoader broke the offset shifting animations that started crolling in unrelated textures.

Existing animations also jump as the offset resets in the animation cycle.

Do what s25client does - animate water and lava by cycling palettes.

Flamefire · 2026-06-30T11:17:31Z

 // array for all palettes
 std::vector<bobPAL> global::palArray(MAXBOBPAL);
+// palette animation data per tileset (indexed by 8-bit tileset bmpArray index)
+std::vector<std::vector<PaletteAnimData>> global::tilesetAnimData;


A bit hard to understand the comment as it only describes 1 vector, not which one nor the other one. maybe add a using PaletteAnimation = std::vector<PaletteAnimData> and name the variable vector<PaletteAnimation> tilesetAnimations or similar

Especially the above

auto& animData = global::tilesetAnimData[tilesetIdx8]; ... for(auto& anim : animData)

is confusingly named if anim is of type PaletteAnimData but animData is not.
You could also turn it around and renamed PaletteAnimData to PaletteAnimation and the vector of that is AnimData to match this code, but not sure if this is clearer.

Anyway such variable vs type naming mismatches are a clear indication of (mis)namings that need to be improved.

morganchristiansson · 2026-07-05T17:19:39Z

Ok I cleaned this up a lot.

I learned libsiedler2 is external project pre-dating RttR and supports writing.
Idk why s25edit reinvents it with CFile should probably outright replace but can be a large change with different data types.
So I guess for now best to add palette animation support to CFile.

Flamefire · 2026-07-05T17:50:26Z

 // array for all palettes
 std::vector<bobPAL> global::palArray(MAXBOBPAL);
+// Palette animation entries keyed by the bmpArray slot of the 8-bit tileset surface.
+std::map<Uint16, std::map<int, PaletteAnimation>> global::paletteAnimations;


Why is this a map of maps? What is the key of the 2nd?

Outer key is tileset id, inner key is palAnimIdx from GDL lua.
Could also use vector[MapType] as outer, but tileset id was more accessible.

Outer key is tileset id, inner key is palAnimIdx from GDL lua. Could also use vector[MapType] as outer, but tileset id was more accessible.

But why is this used/stored? The inner map is effectively used as an expensive vector

Right there's only 3 tilesets, could use separate fields too, and each tileset has only 2 animations, water and lava.
Tho could be possibly change that in the GDL lua.
Can also add a field to the struct in bmpArrar but then every surface/texture gets it. Not sure what's optimal.

You apply the animation to the whole tileset, don't you? So for each animation step you apply all animations for that tilesets. You don't differentiate between water an lava, do you?
So why isn't this a mapping of tileset to list/vector of animations? I might be missing/forgetting something so in that case please remind me exactly where you'd use the 2nd key

Good point, not sure and I will check. I think there are separate palAninIdx in same biome lua file tho. Maybe I'm wrong.

Yep you're right. They are per biome not per sprite/texture.
std::array<std::vector<PaletteAnimation>, 3> paletteAnimations

Also this will be reworked a bit in gl2-terrain also to support it with OpenGL.
s25client precomputes and cycles textures, seems reasonable given 32bit non-paletted textures.

s25edit (current): runtime palette cycling

CRNG animations stored as PaletteAnimation structs

Each frame: rotates the entire 8-bit tileset palette in-place, then blits 8-bit → 32-bit surface

Simple, but requires SDL palette surfaces and can't work with OpenGL textures directly

s25client/RTTR: pre-compute all animation frames at load time

Each terrain type (water, lava) has its own palAnimIdx in the Lua config referencing a specific CRNG entry

ExtractAnimatedTexture extracts the terrain's texture region into a paletted buffer, then generates N pre-rotated copies (one per animation frame) by calling anim.apply() which shifts the palette by one step each iteration

Each frame becomes its own glArchivItem_Bitmap_Raw with a baked palette — no runtime palette work needed

At render time, just cycles texture index: terrainTextures[curIdx].textures[frame]

Naturally OpenGL-compatible since textures hold final RGBA pixel data

The RTTR approach is basically "bake the animation into separate textures at load time" instead of "rotate the palette at runtime." For s25edit's future OpenGL port, that's the direction to look at — though s25edit would still apply all animations globally to the tileset rather than per-terrain, the pre-computation strategy would remove the need for runtime palette surface manipulation.

Flamefire · 2026-07-05T17:52:34Z

+                                       Point16& left, Point16& right, Point16& upper2, Point16& left2, Point16& right2)
 {
-    const auto animOffset = Point16(-texture_move, texture_move);
+    // Offset-shifting animation replaced by palette cycling animation (UpdatePaletteAnimations)


Flamefire · 2026-07-05T17:58:45Z

+static void rotatePaletteRange(SDL_Palette* pal, uint8_t firstClr, int colorCount, int deltaOffset, bool moveUp)
+{
+    std::array<SDL_Color, 256> rotated;
+    memcpy(rotated.data(), pal->colors, sizeof(SDL_Color) * 256);


Avoid sizeof

Suggested change

memcpy(rotated.data(), pal->colors, sizeof(SDL_Color) * 256);

std::copy_n(pal->colors, rotated.size(), rotated.begin());

Actually: You don't use that at all: You set all colors manually and could simply do rotated[i] = pal->colors[firstClr + srcIdx];
Or is this missing something?

Flamefire · 2026-07-05T18:05:27Z

+    uint8_t lastClr = 0;
+    int currentOffset = 0;
+    int lastAppliedOffset = 0;
+    uint32_t lastUpdateTime = 0;


Do you need a fixed size here?

Suggested change

uint32_t lastUpdateTime = 0;

unsigned lastUpdateTime = 0;

Same for rate, Clr makes sense as those are 8bit colors (indices)

Flamefire · 2026-07-05T18:08:06Z

+        if(!anim.isActive || anim.rate == 0)
+            continue;
+
+        const int colorCount = anim.lastClr - anim.firstClr + 1;
+        if(colorCount <= 1)
+            continue;


Wouldn't it make sense to not add "inactive" animations at all?
And verify they are valid before adding: (lastClr>firstClr && rate>0)

not fully handled/answered

morganchristiansson · 2026-07-05T19:24:06Z

#	Feedback	Change
1	Confusing comment on paletteAnimations	Reworded comments in globals.h and globals.cpp
2	Why map of maps?	Already explained in reply (tileset slot → CRNG index)
3	Move tilesetSlot/chunkIdx before CMAP comment	Moved them before /* READ SECOND CHUNK */
4	"AI" comment about offset-shifting	Removed the comment line entirely
5	unsigned return type for tilesetIdxForMapType	Changed Uint16 → unsigned everywhere
6	Default values needed?	Removed all default values from PaletteAnimation struct; fields are now explicitly initialized
7	Avoid sizeof	Replaced memcpy with std::copy_n
8	Use reference instead of pointer for surf32	Changed auto* → auto&, adjusted the blit section
9	Fixed-width types (uint32_t)	Changed uint32_t → unsigned in struct and function
10	Consistent types for time	Now uses unsigned for now, elapsed, lastUpdateTime everywhere
11	Don't add inactive animations	Removed isActive field entirely; now validates rate > 0 && lastClr > firstClr before adding
12	Missing lastAppliedOffset init	Added anim.lastAppliedOffset = 0 in CFile.cpp

morganchristiansson · 2026-07-05T22:17:37Z

I forgot to push? Will push tomorrow

morganchristiansson · 2026-07-06T05:11:44Z

Also removed the special handling of ice floe water animations which is not needed anymore.

morganchristiansson · 2026-07-06T07:42:58Z

Also kind of regretting not using libsiedler2 to read files, but the CFile addition is kinda clean. Maybe best to keep CFile or maybe best to throw it out for libsiedler2.
Can try migration in separate PR or maybe it's unnecessary churn to migrate.

Flamefire reviewed Jun 30, 2026

View reviewed changes

morganchristiansson mentioned this pull request Jun 30, 2026

Use pos from GameDataLoader #55

Open

morganchristiansson force-pushed the palanim branch from 2ca6406 to f3a57cc Compare July 5, 2026 17:18

Flamefire requested changes Jul 5, 2026

View reviewed changes

morganchristiansson force-pushed the palanim branch 2 times, most recently from 923a1bc to a1e596d Compare July 6, 2026 05:11

morganchristiansson mentioned this pull request Jul 6, 2026

OpenGL Terrain editor #59

Open

morganchristiansson force-pushed the palanim branch from 84555ea to 190336a Compare July 6, 2026 17:48

morganchristiansson added 4 commits July 8, 2026 20:45

Palette cycling animation

a9c0570

Address review feedback

7aed1d0

ice floe animation

be12fec

palette animations are per tileset not sprite

b95f756

morganchristiansson force-pushed the palanim branch from 190336a to b95f756 Compare July 8, 2026 18:45

	memcpy(rotated.data(), pal->colors, sizeof(SDL_Color) * 256);
	std::copy_n(pal->colors, rotated.size(), rotated.begin());

Uh oh!

Conversation

morganchristiansson commented Jun 30, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

morganchristiansson commented Jul 5, 2026

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

morganchristiansson Jul 5, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

morganchristiansson Jul 6, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

morganchristiansson commented Jul 5, 2026

Uh oh!

morganchristiansson commented Jul 5, 2026

Uh oh!

morganchristiansson commented Jul 6, 2026

Uh oh!

morganchristiansson commented Jul 6, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

morganchristiansson commented Jun 30, 2026 •

edited

Loading

morganchristiansson Jul 5, 2026 •

edited

Loading

morganchristiansson Jul 6, 2026 •

edited

Loading